Support root issuer generation #14975

stevendpclark · 2022-04-08T17:04:31Z

Add initial very basic support for creating issuers through the new /issuers/generate/root api.

This needs some further refinement/refactoring but I'd like to get some early feedback on the approach.
Testing at the moment for the existing is a bit lacking, more to come.
This is just for root at the moment, not the intermediate endpoint that will most likely be another PR.

- Update all new API endpoints to use the new agreed upon argument names. - issuer_ref & key_ref to refer to existing - issuer_name & key_name for new definitions - Update returned values to always user issuer_id and key_id

- Add utility methods to fetch the issuer_name, issuer_ref, key_name and key_ref arguments from data fields. - Centralize the logic to clean up these inputs and apply various validations to all of them.

cipherboy

Looking good :-) Some minor comments.

builtin/logical/pki/storage.go

builtin/logical/pki/path_root.go

- Use the buildPath convention for the function name instead of common...

…suer} methods - PR feedback, move setting up the default configuration references within the import methods instead of within the writeCaBundle method. This should now cover all use cases of us setting up the defaults properly.

kitography

This is pretty damn good, super impressed by some of the generate tests, have quite a few nits (though not nearly as many as expected on 721 lines of code)

kitography · 2022-04-11T19:26:27Z

builtin/logical/pki/ca_util.go

+	var keyBits int
+
+	switch exportedStr {
+	case "internal":


can we add case "kms" here too?

kitography · 2022-04-11T19:27:50Z

builtin/logical/pki/ca_util.go

 	exportedStr := data.Get("exported").(string)
 	switch exportedStr {
 	case "exported":
 		exported = true
 	case "internal":
+	case "existing":


we switch on the type twice here - here, and then in getKeyTypeAndBitsForRole, could we split that function up instead (to one function for exported, one for internal, one for exsiting, one for kms)?

kitography · 2022-04-11T19:29:47Z

builtin/logical/pki/ca_util.go

+	}
+
+	var pubKey crypto.PublicKey
+	if kmsRequestedFromFieldData(data) {


this looks like part of the case statement? Can we do this, or as a case statement, and not both?

kitography · 2022-04-11T19:31:20Z

builtin/logical/pki/config_util.go


 	"github.com/hashicorp/vault/sdk/logical"
 )

+func isKeyDefaultSet(ctx context.Context, s logical.Storage) (bool, error) {


This doesn't look like it sets the KeyDefault? Key default isn't a set?

Nevermind, I'm an idiot, leaving this as I clearly was confused by it, another person might be too [existsKeyDefault is kinda wordy though]

Definitely not an idiot, it was badly named. Updating to isDefaultKeySet and isDefaultIssuerSet in a hope to make this clearer.

kitography · 2022-04-11T19:35:08Z

builtin/logical/pki/fields.go

+
+	return fields
+}
+


total nit: ordering here (next six functions) is super weird - can all the key be together (and all the issuer together), or pair off NameFields; RefField; NameField

kitography · 2022-04-11T20:12:34Z

builtin/logical/pki/backend_test.go

+	})
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "issuer name already used")
+


what happens if we try generating a root credential with a name which is the Issuer's Key ID? (I'm hoping that this would fail)

Yes that would fail if an end-user were to try to use an existing id for a Name value. Now if they could predict the uuid we would generate in the future though, that would be a bad day...

kitography · 2022-04-11T20:17:13Z

builtin/logical/pki/backend_test.go

+
+	require.NotEqual(t, myKeyId1, myKeyId2)
+	require.Equal(t, myKeyId1, myKeyId3)
+}


key name reusue? issuer name reuse (for non roots?)

Validating that the new CA we generated re-used a key that we had previous generated. Will add a clearer message on this test.

kitography · 2022-04-11T20:23:32Z

builtin/logical/pki/storage_test.go

 	require.NoError(t, err)
 	require.False(t, existing)
 	require.Equal(t, key1.PrivateKey, key1_ref1.PrivateKey)

-	key1_ref2, existing, err := importKey(ctx, s, key1.PrivateKey)
+	key1_ref2, existing, err := importKey(ctx, s, key1.PrivateKey, "ignore-me")


what are we trying to test here? Like, this is a test on a helper function, what are we trying to guarantee to the user?

The "contract" as far as I understand it, is if we determine that the key/issuer already exists no updates occur from the importXXX functions and that the existing flag is set to true. Will add some comments around these tests.

kitography · 2022-04-11T20:29:54Z

builtin/logical/pki/path_fetch_issuers.go

@@ -61,25 +61,17 @@ their identifier and their name (if set).
 )

 func pathGetIssuer(b *backend) *framework.Path {
-	pattern := "issuer/" + framework.GenericNameRegex("ref") + "(/der|/pem)?"
+	pattern := "issuer/" + framework.GenericNameRegex("issuer_ref") + "(/der|/pem)?"


nit: the number of places this is referred to, i would love to see a constant

cipherboy · 2022-04-12T17:37:45Z

Manually merging :-)

cipherboy · 2022-06-20T15:53:10Z

This PR was merged in #15277. See that PR and the relevant docs PR #15238 for more information about this change.

stevendpclark added secret/pki pr/no-changelog rotation labels Apr 8, 2022

stevendpclark requested review from kitography and cipherboy April 8, 2022 17:04

stevendpclark force-pushed the stevendpclark/vault-5628-issuer-generation branch from 57f2df8 to a76ec20 Compare April 8, 2022 17:19

vercel bot temporarily deployed to Preview – vault April 8, 2022 17:19 Inactive

vercel bot temporarily deployed to Preview – vault-storybook April 8, 2022 17:19 Inactive

vercel bot temporarily deployed to Preview – vault April 8, 2022 19:39 Inactive

vercel bot temporarily deployed to Preview – vault-storybook April 8, 2022 19:39 Inactive

stevendpclark force-pushed the stevendpclark/vault-5628-issuer-generation branch from 45b2633 to 240d51c Compare April 8, 2022 19:39

vercel bot temporarily deployed to Preview – vault-storybook April 8, 2022 19:39 Inactive

vercel bot temporarily deployed to Preview – vault April 8, 2022 19:39 Inactive

vercel bot temporarily deployed to Preview – vault April 11, 2022 16:49 Inactive

vercel bot temporarily deployed to Preview – vault-storybook April 11, 2022 16:49 Inactive

stevendpclark added 4 commits April 11, 2022 12:51

Add support root issuer generation

3903eab

Add support for issuer generate intermediate end-point

5f2179f

Update issuer and key arguments to consistent values

e301ef4

- Update all new API endpoints to use the new agreed upon argument names. - issuer_ref & key_ref to refer to existing - issuer_name & key_name for new definitions - Update returned values to always user issuer_id and key_id

Add utility methods to fetch common ref and name arguments

d2f24d7

- Add utility methods to fetch the issuer_name, issuer_ref, key_name and key_ref arguments from data fields. - Centralize the logic to clean up these inputs and apply various validations to all of them.

stevendpclark force-pushed the stevendpclark/vault-5628-issuer-generation branch from 4a34597 to d2f24d7 Compare April 11, 2022 16:51

vercel bot temporarily deployed to Preview – vault April 11, 2022 16:51 Inactive

vercel bot temporarily deployed to Preview – vault-storybook April 11, 2022 16:51 Inactive

stevendpclark marked this pull request as ready for review April 11, 2022 16:53

cipherboy reviewed Apr 11, 2022

View reviewed changes

builtin/logical/pki/storage.go Outdated Show resolved Hide resolved

builtin/logical/pki/path_root.go Outdated Show resolved Hide resolved

stevendpclark added 2 commits April 11, 2022 14:16

Rename common PKI backend handlers

58115c0

- Use the buildPath convention for the function name instead of common...

vercel bot temporarily deployed to Preview – vault April 11, 2022 18:35 Inactive

vercel bot temporarily deployed to Preview – vault-storybook April 11, 2022 18:35 Inactive

kitography reviewed Apr 11, 2022

View reviewed changes

cipherboy mentioned this pull request Apr 11, 2022

Fix /intermediate/set-signed, /config/ca import and other minor test scenarios #14999

Closed

Introduce constants for issuer_ref, rename isKeyDefaultSet...

ce4769a

vercel bot temporarily deployed to Preview – vault April 11, 2022 21:25 Inactive

vercel bot temporarily deployed to Preview – vault-storybook April 11, 2022 21:25 Inactive

stevendpclark mentioned this pull request Apr 12, 2022

Introduce defaultRef constant within PKI #15003

Merged

cipherboy closed this Apr 12, 2022

stevendpclark deleted the stevendpclark/vault-5628-issuer-generation branch April 20, 2022 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support root issuer generation #14975

Support root issuer generation #14975

stevendpclark commented Apr 8, 2022

cipherboy left a comment

kitography left a comment

kitography Apr 11, 2022

kitography Apr 11, 2022

kitography Apr 11, 2022

kitography Apr 11, 2022

kitography Apr 11, 2022

stevendpclark Apr 11, 2022

kitography Apr 11, 2022

kitography Apr 11, 2022

stevendpclark Apr 11, 2022

kitography Apr 11, 2022

stevendpclark Apr 11, 2022

kitography Apr 11, 2022

stevendpclark Apr 11, 2022 •

edited

Loading

kitography Apr 11, 2022

cipherboy commented Apr 12, 2022

cipherboy commented Jun 20, 2022

Support root issuer generation #14975

Support root issuer generation #14975

Conversation

stevendpclark commented Apr 8, 2022

cipherboy left a comment

Choose a reason for hiding this comment

kitography left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevendpclark Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy commented Apr 12, 2022

cipherboy commented Jun 20, 2022

stevendpclark Apr 11, 2022 •

edited

Loading